-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for nested generics #1104
Conversation
ca5f73d
to
fe5c43c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1104 +/- ##
======================================
Coverage 100% 100%
======================================
Files 20 20
Lines 3450 3463 +13
Branches 667 672 +5
======================================
+ Hits 3450 3463 +13
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the generated names being weird, but can we have an example in a test so we're clear.
As this PR stands, an error will be raised if you try to instantiate a GenericModel after just passing TypeVars as the parameters
I'm afraid I don't know exactly what you mean. Could you add an example here so I (and anyone else coming to this PR) is clear what you mean. My suspicion is that, as you say, this can reasonably considered a bug fix, not a change in functionality so is fine here. But's document it in a test and perhaps even in the documentation so everything is clear.
fe5c43c
to
e2fa3d5
Compare
|
||
|
||
class GenericModel(BaseModel): | ||
__slots__ = () | ||
__concrete__: ClassVar[bool] = False | ||
|
||
def __new__(cls, *args: Any, **kwargs: Any) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This was only necessary to prevent instantiation with unspecified parameters.)
partial_model = Model[int, BT] | ||
assert partial_model.__name__ == 'Model[int, BT]' | ||
concrete_model = partial_model[str] | ||
assert concrete_model.__name__ == 'Model[int, BT][str]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I mean by the names are kind of weird.
It would be a (perhaps surprisingly) involved refactor to make it generate the names properly (by that I mean showing Model[int, str]
instead of Model[int, BT][str]
). Not impossible though if you'd prefer we make it work.
|
||
|
||
@skip_36 | ||
def test_partial_specification_instantiation_bounded(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test and the one below demonstrate that the typevars for (partially-)parametrized models are respected properly. This is a big part of why I think it's okay to drop the check for parametrization in __new__
.
2704265
to
abdb1fc
Compare
I think this looks fine, but I'm not going to pretend I've followed every line of the discussion. Does it need any updates to docs? Otherwise can it be merged? |
I think the docs should be updated, but I think it can be a small update. Give me 15 mins and I'll respond here with an estimate of how long it will take to finish. |
971ac39
to
c4df92d
Compare
@samuelcolvin Okay, I've added docs, let me know if you want any changes. |
awesome, thank you. |
* Add support for nested generics * Allow instantiation of unparameterized generics * Add better more partial instantiation tests * Add changes * Add docs
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Change Summary
Modifies
GenericModel
to behave more similarly to standard python generics, including the ability to reference typevars in nested GenericModels, and to make use of partial specifications with new typevars.There are a couple outstanding issues:
As this PR stands, an error will be raised if you try to instantiate a GenericModel after just passing
TypeVar
s as the parameters. Currently, I believe this is allowed, and will perform the same logic that would happen if you were to use aTypeVar
as a field annotation elsewhere (e.g., for purposes of a constraint).TypeVar
was used as the annotation. I think this is similar to what we do forDict
,List
, etc., since thoseTypeVar
s have no constraints and so are basically equivalent toAny
.Dict
, etc., work. But right now that raises an error. I personally would consider this a fix rather than a breaking change, but I want to reach consensus before moving forward.TypeVar
parameters, I just think that is a wrong API choice -- the question is whether preserving the current behavior is sufficiently worthwhile.)The generated names come out a little weird when you make use of partially-specified models. It would be nice to fix this, but since 1) this is already an edge case, 2) type naming is usually not critical, and 3) we already allow you to override
__concrete_name__
, I am happy to leave this to users for now.Related issue number
Closes #967
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)